Skip to content

fix: create workspace_entities/workspace_documents registry and provision Workspace rows - #1503

Draft
seonghobae wants to merge 34 commits into
codex/starlette-testclient-dependencyfrom
fix/workspace-document-registry-migration
Draft

seonghobae wants to merge 34 commits into
codex/starlette-testclient-dependencyfrom
fix/workspace-document-registry-migration

Conversation

@seonghobae

@seonghobae seonghobae commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Current authority — 2026-09-15

Authority invariant

workspace_id is an opaque authenticated claim. Identifier shape is never ownership evidence and HMAC signature verification alone is not membership proof for an arbitrary (organization_id|user_id, workspace_id) pair.

Historical workspace_documents.organization_id IS NULL rows may be admitted only when the same workspace_entities row proves the authenticated tenant owner:

  • organization scope: (workspace_id, organization_id=current_org, owner_user_id IS NULL);
  • personal scope: (workspace_id, organization_id IS NULL, owner_user_id=current_user).

Organization and personal ownership are mutually exclusive registry states.

RED → causal repair lineage

Organization scope:

  • 2ec213476...: initial opaque-workspace RED.
  • eb0bfd5be...: registry-binding expectation and different-organization denial; review found the SQL assertions insufficiently correlated.
  • fea4d5c39448671c7a1a519930f4ef6bbfbe9fad: exact correlated organization predicate RED.
  • 8e63a20a1641cc5e7c53b196405ac942ff53ea9c: real-PostgreSQL migration acceptance.
  • 3b2e5ba5d091147a6add344b1f3ff44b3f40b3d6: 0020_workspace_organization_binding; only one unambiguous persisted document organization can bind a workspace, ambiguous/evidence-free history stays unbound.
  • bc23bbca05a3ae3749c9d1f7e66b5b070b18e211: trusted establishment/HMAC consume-only service semantics.
  • 909dacce1366101bc76a19b3cc329725323cc244: real-PostgreSQL trusted/HMAC/mismatch/concurrency acceptance.
  • a74db4ea4e16d066502817f028d1710dc8d507be: organization NULL-document contract strengthened to require owner_user_id IS NULL in the same registry row.

Personal scope:

  • 58655161228d1216933f87f5db255d9dbff5000c: source-order RED for (workspace_id, organization_id IS NULL, owner_user_id=user_id).
  • c2c1cd41a64d51172b1b2802bcd12ab2412ef5f0: append-only 0021_workspace_personal_owner_binding; no guessed historical user backfill and fail-closed downgrade once provenance exists.
  • 854ad91a49f01dafb3ccd589e65b2ec2eb0539d2: canonical scoped binding service. Trusted OIDC/server/explicit test authority may establish a fully unbound row; HMAC may consume but never claim; insert/CAS semantics prevent competing owner claims.
  • e204ea75d324809215c57de1fb389376039dc817: real-PostgreSQL personal trusted/HMAC/mismatch/concurrency acceptance.

Current integration work:

  • 1345505ccb0db7d5d1c4b1783b1d589aa7079363: maps Workspace.organization_id and Workspace.owner_user_id to the migrated registry schema.
  • c6bd07dc189cca7374bb6771f4abb11dac6efa22: real-PostgreSQL endpoint acceptance for trusted opaque organization/personal uploads, HMAC non-establishment, owner mismatch denial, and correlated historical NULL-document reads. PostgreSQL unavailability is an acceptance failure, not a skip.
  • 6d801e5c671366267c40c5346f547cf87fd8c45e: repairs the older legacy-document acceptance so it uses an opaque workspace plus persisted organization binding instead of treating workspace-<organization_id> spelling as authorization evidence.
  • 602c1447d781583d70ae7f96846810e89e93e5ad: removes identifier-derived _auth_context_owns_its_workspace, routes JSON and binary document uploads through get_or_create_scoped_workspace(...), maps binding-required/conflict failures to one fail-closed 403 without owner enumeration, and makes historical organization/personal reads require correlated workspace_entities ownership evidence on the same opaque workspace row.
  • f7bdcf11c0ddf0bdfb04a28da18d2d06aa710d67: repairs the two legacy successful HMAC upload fixtures by pre-populating their mock registry with persisted workspace-org-acme → org-acme owner evidence.
  • 9151c75568c582c8147cfee6757cd00a9b4d60b7: responds to the fresh exact-head CodeRabbit P1 finding by applying the same persisted registry precondition to test_data_document_actions_are_workspace_scoped_and_intent_only, whose legacy NULL-organization document actions otherwise passed only because MockAsyncSession does not evaluate the SQLAlchemy Workspace EXISTS predicate. Production authorization remains unchanged; HMAC still consumes existing binding and cannot establish ownership.

The comment-only deletions introduced with 602c1447... are intentionally adjudicated rather than reverted: the removed multipart/implementation narration is redundant with typed Form(...), _NON_MATERIALIZABLE_DOCUMENT_STATUSES, explicit 409/415/422 behavior, and executable tests, while the deleted “Bolt Optimization” prose was non-contract performance narration. No executable behavior or security guard was removed by those comment deletions.

Validation-topology repair

This PR currently has zero PR-triggered repository workflow runs on its exact head because its base is a stacked branch and protected develop still filters Application CI/Bandit/Dependency Review/Docker PR events by base branch. Treating downstream #1587 as the fix was circular: #1503 demanded hosted evidence that only a downstream PR could enable.

#1691 is the clean develop-based successor for all eight valid #1587 repository-CI deltas, rebuilt on protected develop with no #1503 domain source or ancestry. It broadens only pull_request base acceptance, retains push/release semantics, adds pinned pgvector PostgreSQL 16, an explicit CI database URL, generated/masked HMAC runtime secret, migration-before-pytest, and executable trigger/PostgreSQL governance contracts. Independent review of the initial successor found the PostgreSQL CI contract outside the backend pytest collection; current exact f985a000... moves it to backend/tests/test_postgres_ci_contract.py and fixes its repository-root calculation without weakening the assertions.

On f985a000..., Application CI, Security Scan, Bandit, Semgrep, CodeQL PR, and Docker have all materialized but remain queued. Application CI's backend/frontend jobs currently have no assigned runner (runner_id=0), so this is an execution-capacity wait state, not evidence that #1503 or #1691 is source-GREEN. #1691 also has no qualifying current-head independent review; its only formal CodeRabbit review is dismissed predecessor evidence from 971f1752....

Do not copy #1691 workflow source into this owner. The correct path is normal protected integration of #1691, ordinary adoption by prerequisite branches, and only then exact-head validation of this domain owner.

Exact-head evidence gate

This head is source-complete for the currently known workspace-ownership RED, but it is not GREEN yet:

  • fix(ci): make stacked PR validation a develop prerequisite #1691 must first be terminal GREEN, independently reviewed and normally integrated so stacked bases can produce repository-local validation without circular dependency;
  • current commit statuses from review bots are not substitutes for protected required contexts;
  • the old CodeRabbit CHANGES_REQUESTED review was dismissed only as historical after all of its inline findings were resolved; it is not a current-head approval;
  • the fresh f7bdcf11... CodeRabbit review found the action-test binding gap and 9151c755... is the repair; therefore a new review of 9151c755... is required;
  • focused unit + migration/binding + endpoint real-PostgreSQL suites must pass on the unchanged final head;
  • qualifying independent post-last-push review is required before Ready/merge.

No identifier-shape backfill, default owner, source-copy, cross-service SQL, mutable dependency, authorization widening, dummy requeue, predecessor evidence transfer, self-approval, force push/destructive rebase, admin bypass, second Gap-ledger writer, or gate weakening is permitted.

Migration and descendant boundary

Canonical owner lineage is 0018_workspace_registry → 0019_email_read_state_repair → 0020_workspace_organization_binding → 0021_workspace_personal_owner_binding.

#1587 remains open/Draft as the historical branch carrying the same eight CI deltas until #1691 is fully validated and normally integrated; it must ordinary-adopt the protected successor rather than be simply closed. #1503 must then obtain its own exact-head evidence. #1486 remains downstream and eventually rechains its calendar-conflict / attachment-UID / email-workspace / correction-rationale / Noema revisions after owner 0021, with no parallel Alembic head or revision-ID collision.

…space rows

Workspace/Document (workspace_entities/workspace_documents) have been
declared in db/models.py since June, but no Alembic migration ever created
them explicitly, and no production write path ever inserted a Workspace
row. A database that incrementally migrated forward before these models
existed never gets the tables (0001's Base.metadata.create_all only
reflects today's model metadata, not a historical snapshot), and even
where the tables exist, /api/data/documents' Document inserts always
violated the workspace_id foreign key since nothing ever created the
referenced Workspace row for a real signed session.

- Add 0018_workspace_registry.py: idempotent (has_table-guarded) creation
  of both tables, matching the current model shape and this repo's
  structured-migration convention.
- Add services/workspace_scope.get_or_create_workspace and wire it into
  both Document-creating endpoints in api/data.py, keyed by the signed
  session's real workspace claim (workspace-<organization_id>, confirmed
  against every other call site that derives it) rather than the model's
  own opaque uuid default.
- Fix two unrelated, independently-discovered bugs blocking the documented
  Alembic path (scripts/migrate_db.py) from ever completing on a genuinely
  fresh database: schema_backfill_sql() and 0011_email_read_state.py both
  still targeted the "emails" table, renamed to "email_records" by
  0011_email_model_reconciliation long ago.
- Add test_workspace_document_migration.py: runs the real
  scripts/migrate_db.py against a disposable Postgres database (never
  create_all) and proves /api/data/documents serves cleanly both from an
  empty database and from one that had already migrated past the point
  where the registry tables would otherwise be missing.

Verified locally against a real PostgreSQL 16 instance: the full Alembic
chain now runs 0001->head cleanly from empty, and the full backend test
suite passes (1836 passed; the 2 remaining failures are a pre-existing,
unrelated is_read NOT NULL smoke-test bug, confirmed present on
unmodified develop before this change).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016ErwZSYW3pm585NiM3Q7aN
@coderabbitai

coderabbitai Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The pull request hardens Alembic migrations, adds workspace registry tables, provisions workspaces during document uploads, and applies organization-aware document filtering with legacy-row handling. It adds unit and PostgreSQL regression coverage for migrations, concurrency, uploads, and access isolation.

Changes

Schema and workspace document integrity

Layer / File(s) Summary
Migration backfills and repairs
backend/alembic/versions/0001_initial_control_plane.py, backend/alembic/versions/0011_email_read_state.py, backend/alembic/versions/0019_email_read_state_repair.py, backend/scripts/bootstrap_db.py, backend/tests/test_alembic_migrations.py, backend/tests/test_bootstrap_db.py, backend/tests/test_email_read_state_migration_postgres.py, backend/tests/test_data_api.py
Schema backfills now skip missing legacy tables. Email read-state changes check table and column existence. Revision 0019_email_read_state_repair restores missing email_records.is_read. Downgrades preserve read state.
Workspace registry and upload provisioning
backend/alembic/versions/0016_document_org_scope.py, backend/alembic/versions/0018_workspace_registry.py, backend/services/workspace_scope.py, backend/api/data.py, backend/tests/test_workspace_document_migration.py
The migration chain creates workspace tables and indexes idempotently. Upload endpoints call get_or_create_workspace, which handles concurrent inserts. PostgreSQL tests cover fresh, upgraded, and concurrent uploads.
Organization-scoped document access
backend/api/data.py, backend/tests/test_data_api.py, backend/tests/test_legacy_document_scope_postgres.py
Document lookups and quality-surface results apply organization filters. Canonical workspace claims may access legacy rows with null organizations. Tests reject documents belonging to another organization and validate legacy-row access.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant DocumentClient
  participant DataAPI
  participant WorkspaceScope
  participant WorkspaceDatabase
  DocumentClient->>DataAPI: Upload document
  DataAPI->>WorkspaceScope: get_or_create_workspace
  WorkspaceScope->>WorkspaceDatabase: Insert workspace with conflict handling
  WorkspaceDatabase-->>WorkspaceScope: Created or existing workspace
  WorkspaceScope-->>DataAPI: Workspace
  DataAPI->>WorkspaceDatabase: Insert document
  WorkspaceDatabase-->>DataAPI: Document record
  DataAPI-->>DocumentClient: Upload response
Loading

Merge Risk: 🟡 Moderate · up to da2d7

This PR adds workspace registry tables and provisions workspaces before document creation, but it is not merge-ready until the downgrade path preserves existing organization assignments. A validly signed but mismatched workspace and organization claim pair could also create inconsistent ownership records, while migration tests currently have configuration-handling and status-validation gaps.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the primary changes: creating the workspace registry tables and provisioning Workspace rows. It is concise, specific, and directly related to the pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/workspace-document-registry-migration

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

PR governance metadata gate is not ready for 9c1851336fa04bcdc77c1c6e531afdb882583af1:

  • Review decision is CHANGES_REQUESTED; address requested changes before merge.
  • Required check strix is CANCELLED on the current head.
  • Current-head CodeRabbit issue comment has blocking warning/failure evidence on 9c18513.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Exact-head audit found and fixed a workspace-provisioning concurrency defect at 31b400c. The former SELECT-then-INSERT allowed simultaneous first uploads for one signed workspace to race on the workspace_entities.workspace_id primary key. Provisioning now uses PostgreSQL ON CONFLICT DO NOTHING with RETURNING and selects the winning row only when another transaction created it.

Verification: real PostgreSQL migration/API suite 3 passed (fresh install, pre-registry upgrade, and 16 concurrent first uploads); focused Data API 15 passed and 1 skipped; Ruff and git diff --check passed.

@coderabbitai review
@opencode-agent

@coderabbitai

coderabbitai Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

@seonghobae I will review the exact-head changes, including the concurrent workspace-provisioning path.

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Follow-up exact-head audit fix at 37bcd6e: workspace_id alone was used by document lookup and Data repository listing even though workspace resources must also retain organization scope. A valid signed session carrying the same workspace identifier under another organization could therefore read or mutate that organization’s document. Both query paths now require workspace_id plus organization_id; regression coverage uses the same workspace with a different explicit organization and proves 404. Combined focused validation: 18 passed, 1 skipped; real PostgreSQL fresh/upgrade/concurrency coverage included; Ruff and git diff --check passed.

@coderabbitai review
@opencode-agent

@seonghobae

Copy link
Copy Markdown
Contributor Author

PR governance metadata gate is not ready라고 띄우는 Workflow가 무슨 소용이 있지? 이슈가 있으면 이슈가 있다고 알리고, GitHub Checks가 실패했으면 실패를 고치기 위한 OpenCode Agent를 contextual-orchestrator 를 orchestrator/free 로 대동해서 띄우는 게 더 생산적이지 않을까? 그리고 OpenCode Agent는 Ready for Review로 넘겨주든 어쩌든 의사결정을 하고.

@coderabbitai

coderabbitai Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

git status --short
git show --no-ext-diff --format=fuller --stat 37bcd6e38dd26c4aaf9af5837d8f920328bcca16
git diff --no-ext-diff 37bcd6e38dd26c4aaf9af5837d8f920328bcca16^ 37bcd6e38dd26c4aaf9af5837d8f920328bcca16 -- backend

printf '\n--- Python file outlines affected by the commit ---\n'
git diff --name-only 37bcd6e38dd26c4aaf9af5837d8f920328bcca16^ 37bcd6e38dd26c4aaf9af5837d8f920328bcca16 -- 'backend/**/*.py' |
while IFS= read -r file; do
  [ -n "$file" ] || continue
  printf '\n### %s\n' "$file"
  ast-grep outline "$file" --items all --view expanded || true
done

printf '\n--- Workspace/document query candidates ---\n'
rg -n -C 4 --glob 'backend/**/*.py' \
  '(Document\.workspace_id|workspace_id\s*==|workspace_id\s*=' backend

Length of output: 35493


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- Scoped helper and document endpoints ---'
sed -n '2470,2520p' backend/api/data.py
sed -n '3150,3395p' backend/api/data.py
sed -n '3900,3985p' backend/api/data.py

printf '%s\n' '--- Mock session query emulation ---'
sed -n '55,205p' backend/tests/test_data_api.py

printf '%s\n' '--- Changed document-scope regression test and neighboring listing test ---'
sed -n '2520,2740p' backend/tests/test_data_api.py

printf '%s\n' '--- Direct Document query statements in data API ---'
ast-grep run --lang python \
  --pattern 'select(Document)' \
  backend/api/data.py || true
ast-grep run --lang python \
  --pattern 'update(Document)' \
  backend/api/data.py || true
ast-grep run --lang python \
  --pattern 'delete(Document)' \
  backend/api/data.py || true

printf '%s\n' '--- Callers of the scoped document helper ---'
rg -n -C 3 '\b_get_workspace_document\s*\(' backend/api/data.py

printf '%s\n' '--- Document workspace and organization predicates ---'
rg -n -C 3 'Document\.(workspace_id|organization_id)\s*==' backend/api/data.py backend/tests/test_data_api.py

Length of output: 32842


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- Document model and registry migration ---'
rg -n -C 10 'class Document\b|organization_id.*(Column|mapped_column)' backend/db/models.py
sed -n '1,280p' backend/alembic/versions/0018_workspace_registry.py

printf '%s\n' '--- All backend Document query and write candidates ---'
rg -n -C 4 --glob 'backend/**/*.py' \
  '(select\(Document\)|update\(Document\)|delete\(Document\)|query\(Document\)|Document\))' \
  backend

printf '%s\n' '--- Organization-collision coverage in document and migration tests ---'
rg -n -i -C 5 \
  '(other_organization|org-rival|organization_id|quality-surface|workspace-org-acme)' \
  backend/tests/test_data_api.py backend/tests/test_workspace_document_migration.py

printf '%s\n' '--- Exact-head test-file changes since the parent commit ---'
git diff --no-ext-diff --check 37bcd6e38dd26c4aaf9af5837d8f920328bcca16^ 37bcd6e38dd26c4aaf9af5837d8f920328bcca16

Length of output: 81549


@seonghobae I found one test coverage issue.

P2 — Test the organization boundary in the repository listing

MockAsyncSession.execute treats a document with organization_id=None as a match for an explicit organization. PostgreSQL does not match that row for Document.organization_id == 'org-acme'. As a result, test_data_quality_surface_includes_workspace_document_assets passes with its doc_owned fixture even though the fixture lacks organization_id.

The test also does not add a document with the same workspace_id and a different explicit organization_id. Therefore, it does not verify the new predicate in get_data_quality_surface.

Set doc_owned.organization_id="org-acme". Add a same-workspace fixture with organization_id="org-rival". Assert that the quality-surface response excludes the rival document. Make the mock require exact organization matching when the query contains an organization predicate.

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

CodeRabbit flagged that test_data_quality_surface_includes_workspace_document_assets
didn't actually exercise the new Document.organization_id predicate added in
37bcd6e: doc_owned had no organization_id set, and MockAsyncSession treats a
None organization_id as matching any query filter (intentionally, to keep
older organization_id-less fixtures usable), so the test passed regardless of
whether the real query filtered by organization at all.

Give doc_owned its real organization_id and add doc_other_org: same
workspace_id, different organization_id. Verified this reproduces the gap
(reverting the organization_id predicate in get_data_quality_surface turns
this red with object_count 2 instead of 1) and passes with the fix restored.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016ErwZSYW3pm585NiM3Q7aN

Copy link
Copy Markdown
Contributor Author

Fixed the P2 test-coverage gap CodeRabbit flagged at bfe347e7: test_data_quality_surface_includes_workspace_document_assets now gives doc_owned its real organization_id and adds doc_other_org (same workspace_id, different organization_id), so the test actually exercises the Document.organization_id predicate added in 37bcd6e3 rather than passing regardless of it.

Verified: reverting that predicate in get_data_quality_surface turns this test red (object_count: 2 instead of 1); restoring it and running the full backend suite locally against real PostgreSQL 16 stays green (1837 passed, the same 2 pre-existing unrelated is_read failures noted in the PR description, ruff clean).


Generated by Claude Code

@seonghobae
seonghobae marked this pull request as ready for review September 1, 2026 04:16
devin-ai-integration[bot]

This comment was marked as resolved.

Devin Review correctly identified two real gaps in 0018_workspace_registry:

1. A database that truly never had workspace_entities/workspace_documents
   (one whose own 0001_initial_control_plane ran before these models
   existed, and has only applied incremental migrations since) crashes on
   0016_document_org_scope with NoSuchTableError, because 0016 calls
   inspector.get_columns() unconditionally and it sits before 0018 in the
   chain. My existing regression test only dropped the tables after 0017,
   which never exercised this because 0001 always recreates them via live
   create_all for a genuinely fresh test database -- masking the real bug.
   Reproduced directly (migrate to 0015, drop the tables, continue to head)
   and confirmed the crash; 0016 is now has_table-guarded like the rest of
   this repo's idempotent migrations, and the regression test's pre-registry
   boundary moved from 0017 to 0015 so it actually crosses 0016 with the
   tables absent.

2. 0018's downgrade unconditionally dropped both tables, including when its
   own upgrade was a no-op because they already existed -- so a rollback on
   any database would destroy workspace_documents.document_content (real
   uploaded content, not rebuildable derived state like most other tables
   this repo's migrations manage). Made downgrade a documented no-op,
   matching the same judgment call 0001_initial_control_plane already makes
   for the same reason.

Verified: the reproduction above now completes cleanly end-to-end through
/api/data/documents; full backend suite still 1837 passed (same 2
pre-existing unrelated is_read failures), ruff clean.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016ErwZSYW3pm585NiM3Q7aN

Copy link
Copy Markdown
Contributor Author

Thanks, this review caught a real gap. Addressed at 317c721e:

Finding 1 (🔴 historical upgrades never reach registry creation) — confirmed and fixed. Reproduced directly against real PostgreSQL: migrated a fresh database to 0015_merge_newsdom_email_heads, dropped workspace_entities/workspace_documents to simulate a database whose own 0001_initial_control_plane predates these models, then continued to head — 0016_document_org_scope crashed with NoSuchTableError exactly as you said, before 0018_workspace_registry ever ran. My original regression test never caught this because it only dropped the tables after 0017 (by which point 0016 had already run against tables that existed via create_all's live metadata) — that masked the real bug. 0016 is now has_table-guarded like the rest of this repo's idempotent migrations, and the test's pre-registry boundary moved from 0017 to 0015 so it actually crosses 0016 with the tables absent. Confirmed the same reproduction now completes cleanly end-to-end through /api/data/documents.

Finding 2 (📝 fresh migration uses current metadata) — already disclosed, both in the PR description and in 0018's own docstring. No action needed.

Finding 3 (🔴 rollback deletes pre-existing workspace data) — fixed. 0018's downgrade unconditionally dropped both tables even when its own upgrade was a no-op (tables already existed). Made it a documented no-op instead, matching the same judgment call 0001_initial_control_plane already makes for the same reason (document_content holds real uploaded content, not rebuildable derived state — production rollbacks should restore from backup).

Finding 4 (🟡 legacy organization documents disappear) — investigated, not changing. This can't currently manifest: every Document-creating code path always sets organization_id=auth_context.organization_id at insert time, and before this PR no Document row could ever be successfully created in any real deployment (the FK-violation bug this PR fixes meant every upload 500'd). So there's no legacy NULL-organization_id row for a real org session to lose access to today. Personal-scope docs (organization_id=NULL, created by a personal-scope session) stay correctly reachable, since Document.organization_id == auth_context.organization_id becomes IS NULL for a personal-scope session's own None. I'd rather not loosen the predicate defensively — doing so would reopen the exact cross-tenant read 37bcd6e3 closed (a NULL-organization_id row in a shared workspace becoming visible to any org sharing that workspace_id) for a scenario that isn't currently reachable. Happy to revisit if a real write path is ever added that can leave organization_id unset for an org-scoped document.

Full backend suite after this fix: 1837 passed (same 2 pre-existing unrelated is_read failures noted earlier in the thread), ruff clean.


Generated by Claude Code

seonghobae and others added 2 commits September 1, 2026 13:41
test_legacy_document_scope_postgres.py (e05f1b3) reproduced Devin Review's
Finding 4 against real PostgreSQL: 0016_document_org_scope left existing
workspace_documents.organization_id unbackfilled (NULL), and the strict
Document.organization_id == auth_context.organization_id predicate added in
37bcd6e made such a row invisible even to the organization that actually
owns its workspace.

Add _document_organization_filter: an exact organization_id match, OR a NULL
organization_id, but only when the requesting session's own
workspace_id/organization_id pairing is the canonical
workspace-<organization_id> (or workspace-<user_id>) derivation -- not an
internally inconsistent claim. This is what keeps the cross-tenant boundary
37bcd6e added intact: a session whose workspace_id doesn't match what its
own organization_id would derive gets the strict, no-NULL-fallback check,
so a forged or malformed claim pairing still can't read a same-workspace
document under a different organization.

Wired into both _get_workspace_document and get_data_quality_surface's
Document query.

Verified: test_legacy_document_scope_postgres.py now passes (was failing on
this branch's previous commit). Full backend suite: 1838 passed (same 2
pre-existing unrelated is_read failures noted earlier in this PR), ruff
clean.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016ErwZSYW3pm585NiM3Q7aN

Copy link
Copy Markdown
Contributor Author

Picked up test_legacy_document_scope_postgres.py (e05f1b34) — good reproduction, and it changes my earlier call on Devin's Finding 4. I'd reasoned that a NULL-organization_id document couldn't currently arise from any real app write path, so I left it unfixed; this test is right that the data model allows it regardless (the column is nullable, and nothing guarantees only the app's own endpoints ever touch it), so it's worth hardening now rather than waiting for a write path that needs it.

Fixed at 5054a8e7. _document_organization_filter in api/data.py: exact organization_id match, OR a NULL organization_id — but the NULL fallback only applies when the requesting session's own workspace_id/organization_id pairing is the canonical workspace-<organization_id> (or workspace-<user_id>) derivation. That's what keeps both assertions in the new test correct: the matching-org session (self-consistent pairing) reaches the legacy row; the other-org session using the same workspace_id but a different, non-derivable organization_id (an internally inconsistent claim pairing, same class of scenario 37bcd6e3 defends against) still gets refused with the strict, no-fallback check — so a same-workspace document that already has an explicit conflicting organization_id stays protected, and a forged/malformed claim pairing can't use the NULL fallback to read across tenants either.

Wired into both _get_workspace_document and get_data_quality_surface's Document query.

test_legacy_document_scope_postgres.py now passes (confirmed it was failing against the prior commit first). Full backend suite: 1838 passed (same 2 pre-existing unrelated is_read failures noted earlier in the thread), ruff clean.


Generated by Claude Code

devin-ai-integration[bot]

This comment was marked as resolved.

Copy link
Copy Markdown
Contributor Author

Migration overlap / historical-upgrade finding on current head 5054a8e73b70ef542bcee65f5a232f320b152771:

0011_email_read_state is a parallel historical branch (down_revision = 0009_project_graph_projection) later merged by 0014_merge_email_read_state. Retargeting that revision from legacy emails to current email_records is unsafe for an actually old database whose branch has emails but not yet email_records: _has_column(inspector, "email_records", "is_read") can inspect a table that does not exist, and even when it exists it changes the historical revision's ownership semantics.

PR #1502 already carries the narrower compatibility contract: preserve op.add_column("emails", ...) / op.drop_column("emails", ...), but guard both with sa.inspect(op.get_bind()).has_table("emails") so a fresh current-schema install no-ops while a genuine legacy database still receives the historical mutation. Its backend/tests/test_alembic_migrations.py::test_email_read_state_legacy_table_guard_is_reversible pins that behavior.

Please preserve that legacy-table guard rather than the email_records retarget when reconciling the overlapping files. Required RED acceptance before changing production: exercise 0011_email_read_state.upgrade() against a schema where legacy emails exists and email_records does not; it must add is_read to emails without inspecting/mutating a nonexistent current table. Fresh-current-schema migration must still complete as a no-op. This is independent of the workspace registry fix and should not broaden this PR's migration authority.

PR #1502 (opened in parallel, ~5 minutes before this one) independently
diagnosed the same underlying gap from the same root cause -- running the
real backend suite against actual PostgreSQL for the first time -- and found
something this PR didn't: app-ci.yml's backend job has never had a Postgres
services: container, so every @pytest.mark.postgres test (including all the
new ones in this PR) has always silently skipped in real CI. It ships the
authoritative fix for that, plus its own version of the 0011_email_read_state
/bootstrap_db.py fix, with a pinned contract test.

Landing two different versions of the same files from two open PRs would
conflict. Adopt #1502's exact pattern for the overlapping files instead of
this PR's earlier approach:

- 0011_email_read_state.py: has_table("emails")-guarded no-op, keeping the
  original "emails" target, rather than retargeting to "email_records".
  (0011_email_model_reconciliation's own docstring clarifies no migration
  ever renamed "emails" to "email_records" for a real managed database --
  email_records was the actual table name since inception; "emails" was
  only ever a stale copy-pasted string. Both approaches are safe in
  practice, so there's no reason to diverge from the already-tested pattern.)
- bootstrap_db.py / 0001_initial_control_plane.py: schema_backfill_sql()'s
  callers now go through execute_schema_backfill(), which skips the legacy
  ix_emails_owner_date statement via identity-matching a LEGACY_EMAILS_INDEX
  sentinel rather than this PR's simpler unconditional deletion.
- test_alembic_migrations.py: contract test now asserts
  execute_schema_backfill, plus #1502's own
  test_email_read_state_legacy_table_guard_is_reversible pinning the
  reconciled 0011 file's shape.
- test_bootstrap_db.py / test_data_api.py: the 4 raw SQL
  `INSERT INTO email_records` smoke-seeding call sites now set is_read
  explicitly (Python-side ORM default only, no DB server default, so real
  Postgres rejects the omission) -- the exact bug flagged as a follow-up
  earlier in this PR's own investigation.

Re-verified end-to-end: fresh-database migration to head, and the true
historical-database reproduction (migrate to 0015, drop the workspace
registry tables, continue to head crossing both 0011_email_read_state and
0016_document_org_scope) both still complete cleanly. Full backend suite:
1841 passed, 0 failed, 3 skipped (up from 1838/2 failed -- the last 2
pre-existing failures are now fixed too), ruff clean.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016ErwZSYW3pm585NiM3Q7aN

Copy link
Copy Markdown
Contributor Author

Reconciled at 6fe1de1d — thanks for the pointer to #1502.

Adopted its exact pattern for the overlapping files instead of my earlier approach:

  • 0011_email_read_state.py: has_table("emails")-guarded no-op, keeping the original "emails" target, rather than retargeting to "email_records".
  • bootstrap_db.py/0001_initial_control_plane.py: execute_schema_backfill() with the LEGACY_EMAILS_INDEX identity-matched sentinel, rather than my simpler unconditional deletion.
  • test_alembic_migrations.py: pulled in test_email_read_state_legacy_table_guard_is_reversible to pin the reconciled shape.
  • Also absorbed fix(ci): add Postgres service container to backend job, close fresh-install migration gap #1502's fix for the 4 raw-SQL is_read NOT NULL smoke-seeding failures in test_bootstrap_db.py/test_data_api.py — this is the exact bug I'd flagged as a follow-up earlier in this thread, so no reason to leave it half-fixed here now that these files were already being touched.

For what it's worth, checking 0011_email_model_reconciliation's own docstring: no migration ever actually renamed emails to email_records for a real managed database — email_records was the real table name since inception, and "emails" was only ever a stale copy-pasted string in these two spots. So both approaches are safe in practice; no reason to diverge from the already-tested pattern in #1502.

Re-verified end-to-end against real PostgreSQL: fresh-database migration to head, and the true historical-database reproduction (migrate to 0015, drop the workspace registry tables, continue to head crossing both 0011_email_read_state and 0016_document_org_scope) both still complete cleanly. Full backend suite: 1841 passed, 0 failed, 3 skipped (up from 1838/2 failed — the last 2 pre-existing failures are gone too), ruff clean.

Also worth flagging explicitly since it matters for this PR too: #1502's real finding is that app-ci.yml's backend job has never had a Postgres services: container, so every @pytest.mark.postgres test — including all the new ones added here (test_workspace_document_migration.py, test_legacy_document_scope_postgres.py) — has always silently skipped in actual CI, not just run green. Everything in this PR has been validated locally against real PostgreSQL 16, but until #1502 lands, none of it runs for real in CI either.


Generated by Claude Code

devin-ai-integration[bot]

This comment was marked as resolved.

Devin Review found two real gaps in the has_table("emails")-only pattern
adopted from PR #1502 in the previous commit:

1. A genuinely historical database -- one whose own 0001 ran before
   is_read was added to the Email model, so it has email_records without
   is_read -- silently never gets the column: has_table("emails") is False
   (per 0011_email_model_reconciliation's docstring, no managed database
   ever really had a table literally named "emails"), so upgrade() returned
   without touching email_records at all. Reproduced directly: migrated to
   0009, dropped email_records.is_read to simulate that historical state,
   continued to head with the has_table("emails")-only version -- it
   completed with no error, but is_read was permanently missing. Confirmed
   the same reproduction now correctly adds is_read to email_records.

2. Not idempotent: a legacy "emails" table that already has is_read (e.g.
   from a partial/earlier application) made upgrade() crash with a
   duplicate-column error, since it only checked has_table before calling
   op.add_column. Reproduced directly (manually created an "emails" table
   with is_read already present, migrated to head) and confirmed it no
   longer crashes.

Now checks both "email_records" (the table that actually matters) and
"emails" (defensive, in case a real one somehow exists), guarded by column
existence via the same _has_column helper this repo's other migrations
already use, so upgrade/downgrade are safely idempotent either way.

This diverges from PR #1502's exact pinned file shape (its
test_email_read_state_legacy_table_guard_is_reversible asserted the
has_table-only version byte-for-byte), so updated this PR's own contract
test to check for the corrected shape instead of matching that exact text.
Worth flagging on #1502 too, since the same gaps apply to its own version
of this file if it hasn't already been fixed there.

Full backend suite: 1841 passed, 0 failed, 3 skipped, ruff clean.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016ErwZSYW3pm585NiM3Q7aN

Copy link
Copy Markdown
Contributor Author

Two real gaps in the pattern adopted from #1502 — fixed at 053c066c:

"Existing email databases miss read state" (🔴) — confirmed and fixed. Reproduced directly: migrated a fresh database to 0009, dropped email_records.is_read to simulate a database whose own 0001 ran before is_read was added to the Email model, then continued to head with the has_table("emails")-only version — it completed with no error, but is_read was silently, permanently missing (since no managed database ever really had a table literally named "emails", so that guard was always false and upgrade() always returned without touching email_records at all). Confirmed the same reproduction now correctly adds is_read.

"Legacy read-state migration is not idempotent" (🟡) — confirmed and fixed. Reproduced: manually created an "emails" table with is_read already present, migrated to head — the old version crashed with a duplicate-column error (only checked has_table, not column existence, before op.add_column). Confirmed it no longer crashes.

Fix now checks both "email_records" (the table that actually matters) and "emails" (defensive), guarded by column existence via the same _has_column helper this repo's other migrations already use — safely idempotent either way.

This diverges from #1502's exact pinned file shape for 0011_email_read_state.py (its test_email_read_state_legacy_table_guard_is_reversible asserts the has_table-only version byte-for-byte), so I updated this PR's own contract test to check for the corrected shape instead. Worth flagging on #1502 directly too, since the same two gaps apply to its own version of this file if it hasn't been independently caught there — happy to leave that to you/that PR rather than commenting there myself, since you're driving both.

"Raw index DDL bypasses governance" (🔍) — investigated, not changing. LEGACY_EMAILS_INDEX lives in bootstrap_db.py, not an Alembic migration file — CLAUDE.md explicitly documents this script as "the local/dev-only schema compatibility path; Alembic history is authoritative," distinct from the "Alembic migrations use structured operations" convention that governs alembic/versions/*.py. Every other statement in schema_backfill_sql() (~80 of them, predating this PR) already uses raw text() SQL in exactly this style, so singling out this one line would be inconsistent with the rest of the file rather than a real governance gap.

Re-verified end-to-end against real PostgreSQL for all three scenarios above. Full backend suite: 1841 passed, 0 failed, 3 skipped, ruff clean.


Generated by Claude Code

devin-ai-integration[bot]

This comment was marked as resolved.

Devin Review found the same ownership-ambiguity problem in this migration's
downgrade that 0018_workspace_registry's downgrade already had (fixed
earlier in this PR): a fresh database's email_records.is_read comes from
0001's live Base.metadata.create_all, not from 0011_email_read_state, so
there is no way for downgrade() to tell "this revision added the column"
apart from "the baseline already had it" -- and is_read holds real
per-message read/unread state, not rebuildable derived data.

Reproduced directly against real PostgreSQL: migrated a fresh database to
head, then ran alembic downgrade to 0009 -- the previous op.drop_column
version silently deleted email_records.is_read and its data. Made downgrade
a documented no-op instead, matching the same judgment call already applied
to 0001_initial_control_plane and 0018_workspace_registry.

Also added backend/tests/test_email_read_state_migration_postgres.py:
permanent real-Postgres coverage for all three scenarios this migration
must handle (historical email_records missing is_read gets it added;
idempotent against a legacy "emails" table that already has it; downgrade
does not destroy a fresh database's read state), addressing Devin's
separate note that the existing contract test's string-matching assertions
can't detect a destructive downgrade or prove idempotence. Confirmed the
downgrade test fails red against the reverted (destructive) version before
restoring the fix.

Full backend suite: 1844 passed, 0 failed, 3 skipped, ruff clean.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016ErwZSYW3pm585NiM3Q7aN

Copy link
Copy Markdown
Contributor Author

"Fresh database downgrades lose read state" (🔴) — confirmed and fixed at d3020ca7. Same ownership-ambiguity class as 0018_workspace_registry's downgrade, fixed earlier: a fresh database's email_records.is_read comes from 0001's live create_all, not from 0011_email_read_state, so downgrade() can't tell which one is responsible for the column. Reproduced directly: migrated a fresh database to head, ran alembic downgrade to 0009 — the previous version silently dropped is_read and its data. Made downgrade() a documented no-op, matching the same judgment call already applied to 0001_initial_control_plane and 0018_workspace_registry.

"Migration behavior remains untested" (🔍) — addressed. Added backend/tests/test_email_read_state_migration_postgres.py: permanent real-Postgres coverage for all three scenarios this migration has to handle (historical email_records missing is_read gets it added; idempotent against a legacy "emails" table that already has it; downgrade doesn't destroy a fresh database's read state). Confirmed the new downgrade test fails red against the reverted destructive version before restoring the fix.

Full backend suite: 1844 passed, 0 failed, 3 skipped, ruff clean.


Generated by Claude Code

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

Please review exact head 909dacce1366101bc76a19b3cc329725323cc244 only. Focus on 0020_workspace_organization_binding, get_or_create_bound_workspace, the two new real-PostgreSQL acceptance files, concurrency/CAS behavior, migration idempotence, and whether any path can bind or expose an opaque workspace across organizations. The remaining ORM/API structural RED is intentional and is not a request to weaken the test.

@coderabbitai

coderabbitai Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

head="909dacce1366101bc76a19b3cc329725323cc244"
parent="$(git rev-parse "${head}^")"

printf '%s\n' '--- Exact revision identity ---'
git rev-parse HEAD
git show -s --format='commit=%H%nparent=%P%nsubject=%s%n%n%B' "$head"
test "$(git rev-parse HEAD)" = "$head"

printf '%s\n' '--- Exact changed paths and source-order diff ---'
git diff --name-status "$parent" "$head"
git diff --check "$parent" "$head"
git diff --no-ext-diff --unified=100 "$parent" "$head"

printf '%s\n' '--- Changed Python outlines ---'
git diff --name-only "$parent" "$head" -- 'backend/**/*.py' |
while IFS= read -r file; do
  [ -n "$file" ] || continue
  printf '\n### %s\n' "$file"
  ast-grep outline "$file" --items all --view expanded || true
done

printf '%s\n' '--- Migration chain and workspace organization migration ---'
rg -n -C 4 --glob 'backend/alembic/versions/*.py' \
  '^(revision|down_revision|branch_labels|depends_on)\s*=' backend/alembic/versions
fd -a '0020_workspace_organization_binding.py' backend/alembic/versions -x sh -c '
  echo "--- $1 ---"
  cat -n "$1"
' sh {}

printf '%s\n' '--- Workspace model, bound provisioning, and API callers ---'
rg -n -C 15 --glob 'backend/**/*.py' \
  'class Workspace\b|def get_or_create_(bound_)?workspace\b|get_or_create_(bound_)?workspace\s*\(|Workspace\.(workspace_id|organization_id)|insert\(Workspace\)|select\(Workspace\)' \
  backend

printf '%s\n' '--- Document authorization filters and access call sites ---'
rg -n -C 10 --glob 'backend/api/data.py' \
  'def (_get_workspace_document|_auth_context_owns_its_workspace|_document_organization_filter)|_document_organization_filter\(|_get_workspace_document\(|Document\.(workspace_id|organization_id)|Workspace\.(workspace_id|organization_id)' \
  backend/api/data.py

printf '%s\n' '--- New and related PostgreSQL acceptance tests ---'
git diff --name-only "$parent" "$head" -- 'backend/tests/**/*.py' |
while IFS= read -r file; do
  [ -n "$file" ] || continue
  printf '\n### %s\n' "$file"
  cat -n "$file"
done

printf '%s\n' '--- All opaque-workspace and binding references in tests ---'
rg -n -C 8 --glob 'backend/tests/**/*.py' \
  'opaque|bound_workspace|organization_binding|tenant-space|workspace_entities|organization_id|concurr|conflict' \
  backend/tests

printf '%s\n' '--- Static syntax parse for exact changed Python files ---'
git diff --name-only "$parent" "$head" -- 'backend/**/*.py' |
xargs -r python -m py_compile

Length of output: 187970


🏁 Script executed:

#!/bin/bash
set -euo pipefail

head="909dacce1366101bc76a19b3cc329725323cc244"
parent="$(git rev-parse "${head}^")"

printf '%s\n' '--- Exact changed paths ---'
git diff --name-status "$parent" "$head"

printf '%s\n' '--- Exact migration ---'
fd -a '0020_workspace_organization_binding.py' backend/alembic/versions -x sh -c 'cat -n "$1"' sh {}

printf '%s\n' '--- Workspace model ---'
rg -n -A80 -B10 'class Workspace\b' backend/db/models.py

printf '%s\n' '--- Workspace provisioning service ---'
fd -a 'workspace_scope.py' backend/services -x sh -c 'cat -n "$1"' sh {}

printf '%s\n' '--- Bound provisioning callers ---'
rg -n -C 10 --glob 'backend/**/*.py' \
  '\bget_or_create_bound_workspace\s*\(|\bget_or_create_workspace\s*\(' backend

printf '%s\n' '--- New exact-head acceptance test files ---'
git diff --name-only "$parent" "$head" -- 'backend/tests/**/*.py' |
while IFS= read -r file; do
  [ -n "$file" ] || continue
  echo "--- $file ---"
  cat -n "$file"
done

printf '%s\n' '--- Current document scope implementation ---'
sed -n '2460,2555p' backend/api/data.py
sed -n '3935,4010p' backend/api/data.py

printf '%s\n' '--- All direct Workspace database operations ---'
rg -n -C 8 --glob 'backend/**/*.py' \
  '(insert\(Workspace\)|select\(Workspace\)|update\(Workspace\)|delete\(Workspace\)|Workspace\()' \
  backend

printf '%s\n' '--- Migration test references for 0020 ---'
rg -n -C 10 --glob 'backend/tests/**/*.py' \
  '0020_workspace_organization_binding|workspace organization binding|workspace_entities.*organization_id|organization_id.*workspace_entities' \
  backend/tests

Length of output: 51020


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- Complete bound-workspace PostgreSQL acceptance test ---'
cat -n backend/tests/test_workspace_scope_binding_postgres.py

printf '%s\n' '--- Workspace-related PostgreSQL test candidates ---'
fd -a --type f . backend/tests |
grep -E 'workspace|document.*scope|migration.*postgres' |
sort

printf '%s\n' '--- Exact 0020 migration acceptance references and surrounding functions ---'
rg -n -C 25 --glob 'backend/tests/*.py' \
  '0020_workspace_organization_binding|workspace organization binding|unambiguous.*binding|ambiguous.*workspace|workspace_entities.*organization_id' \
  backend/tests

printf '%s\n' '--- Service SQL operation shapes ---'
ast-grep run --lang python --pattern 'await session.execute($STMT)' backend/services/workspace_scope.py
ast-grep run --lang python --pattern '$STMT.on_conflict_do_nothing($$$ARGS)' backend/services/workspace_scope.py
ast-grep run --lang python --pattern '$STMT.values($$$ARGS)' backend/services/workspace_scope.py

printf '%s\n' '--- Migration operation shapes ---'
ast-grep run --lang python --pattern 'connection.execute($STMT)' backend/alembic/versions/0020_workspace_organization_binding.py
ast-grep run --lang python --pattern 'op.$METHOD($$$ARGS)' backend/alembic/versions/0020_workspace_organization_binding.py

printf '%s\n' '--- Revision 0020 files and changed-line check ---'
git show --stat --oneline 909dacce1366101bc76a19b3cc329725323cc244
git diff --check 909dacce1366101bc76a19b3cc329725323cc244^ 909dacce1366101bc76a19b3cc329725323cc244

Length of output: 28429


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- Complete workspace-organization migration PostgreSQL acceptance test ---'
cat -n backend/tests/test_workspace_organization_binding_migration_postgres.py

printf '%s\n' '--- Migration runner revision selection and 0019/0020 relationship ---'
fd -a 'migrate_db.py' backend/scripts -x sh -c 'cat -n "$1"' sh {}
rg -n -C 8 --glob 'backend/alembic/versions/*.py' \
  'revision = "(0018_workspace_registry|0019_email_read_state_repair|0020_workspace_organization_binding)"|down_revision =' \
  backend/alembic/versions

printf '%s\n' '--- Workspace-table foreign keys and organization indexes from migration history ---'
rg -n -C 8 --glob 'backend/alembic/versions/*.py' \
  'workspace_entities|workspace_documents|ix_workspace_entities_organization_id' \
  backend/alembic/versions

printf '%s\n' '--- Test file syntax parse and exact test definitions ---'
python -m py_compile backend/tests/test_workspace_scope_binding_postgres.py backend/tests/test_workspace_organization_binding_migration_postgres.py
ast-grep outline backend/tests/test_workspace_scope_binding_postgres.py --items all --view expanded
ast-grep outline backend/tests/test_workspace_organization_binding_migration_postgres.py --items all --view expanded

Length of output: 73492


@seonghobae I reviewed exact head 909dacce1366101bc76a19b3cc329725323cc244 only.

I found no new actionable issues.

0020_workspace_organization_binding binds a workspace only when its historical documents contain one distinct non-null organization. It leaves ambiguous and evidence-free workspaces unbound. Its repeat-upgrade contract preserves that fail-closed state.

get_or_create_bound_workspace() uses an insert-or-ignore operation and a conditional NULL-binding update. It then reads the persisted binding. A competing organization cannot replace the binding. It receives WorkspaceOrganizationConflict.

The new PostgreSQL acceptance test covers these cases:

  • HMAC cannot establish an unbound workspace binding.
  • A trusted verifier can establish a binding.
  • HMAC can reuse a matching existing binding.
  • A different organization cannot claim the same opaque workspace.
  • Two trusted concurrent claims produce one binding winner and one conflict.

The migration acceptance test covers unambiguous backfill, ambiguous evidence, no evidence, and repeat migration behavior.

git diff --check and Python syntax parsing passed. I did not execute the PostgreSQL tests in this review environment.

You are interacting with an AI system.

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

Please review only current exact head e204ea75d324809215c57de1fb389376039dc817. Focus on the new personal opaque-workspace ownership invariant, 0021_workspace_personal_owner_binding, concurrent CAS semantics in get_or_create_scoped_workspace, and the real-PostgreSQL acceptance. The ORM/API integration is intentionally still RED and must not be mistaken for merge readiness.

@coderabbitai

coderabbitai Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

head="e204ea75d324809215c57de1fb389376039dc817"
parent="$(git rev-parse "${head}^")"

printf '%s\n' '--- Exact revision identity ---'
git rev-parse HEAD
git show -s --format='commit=%H%nparent=%P%nsubject=%s%n%n%B' "$head"
test "$(git rev-parse HEAD)" = "$head"

printf '%s\n' '--- Exact changed paths and diff ---'
git diff --name-status "$parent" "$head"
git diff --check "$parent" "$head"
git diff --no-ext-diff --unified=100 "$parent" "$head"

printf '%s\n' '--- Changed Python outlines ---'
git diff --name-only "$parent" "$head" -- 'backend/**/*.py' |
while IFS= read -r file; do
  [ -n "$file" ] || continue
  printf '\n### %s\n' "$file"
  ast-grep outline "$file" --items all --view expanded || true
done

printf '%s\n' '--- Revision graph and 0021 migration ---'
rg -n -C 4 --glob 'backend/alembic/versions/*.py' \
  '^(revision|down_revision|branch_labels|depends_on)\s*=' backend/alembic/versions
fd -a '0021_workspace_personal_owner_binding.py' backend/alembic/versions -x sh -c '
  echo "--- $1 ---"
  cat -n "$1"
' sh {}

printf '%s\n' '--- Workspace model and scoped provisioning service ---'
rg -n -C 20 --glob 'backend/db/models.py' \
  'class Workspace\b|organization_id|owner|personal' backend/db/models.py
fd -a 'workspace_scope.py' backend/services -x sh -c '
  echo "--- $1 ---"
  cat -n "$1"
' sh {}

printf '%s\n' '--- Scoped provisioning call sites and workspace database operations ---'
rg -n -C 10 --glob 'backend/**/*.py' \
  '\bget_or_create_scoped_workspace\s*\(|\bget_or_create_bound_workspace\s*\(|\bget_or_create_workspace\s*\(|(insert|select|update)\(Workspace\)|Workspace\.(workspace_id|organization_id|owner)' \
  backend

printf '%s\n' '--- Changed and related PostgreSQL acceptance files ---'
git diff --name-only "$parent" "$head" -- 'backend/tests/**/*.py' |
while IFS= read -r file; do
  [ -n "$file" ] || continue
  printf '\n### %s\n' "$file"
  cat -n "$file"
done
rg -n -C 12 --glob 'backend/tests/**/*.py' \
  'personal.*workspace|personal_owner|scoped_workspace|get_or_create_scoped_workspace|opaque|concurr|CAS|owner' \
  backend/tests

printf '%s\n' '--- Syntax parse of exact changed Python files ---'
git diff --name-only "$parent" "$head" -- 'backend/**/*.py' |
xargs -r python -m py_compile

Length of output: 196501


🏁 Script executed:

#!/bin/bash
set -euo pipefail

head="e204ea75d324809215c57de1fb389376039dc817"
parent="$(git rev-parse "${head}^")"

printf '%s\n' '--- Exact changed-path list ---'
git diff --name-status "$parent" "$head"

printf '%s\n' '--- Exact patch only ---'
git diff --no-ext-diff --unified=60 "$parent" "$head" -- \
  backend/alembic/versions \
  backend/db/models.py \
  backend/services/workspace_scope.py \
  backend/tests/test_workspace_personal_scope_binding_postgres.py \
  backend/tests/test_workspace_personal_owner_binding_migration_postgres.py \
  backend/tests/test_workspace_scope_binding_postgres.py \
  backend/tests/test_workspace_organization_binding_migration_postgres.py

printf '%s\n' '--- 0021 migration ---'
fd -a '0021_workspace_personal_owner_binding.py' backend/alembic/versions -x cat -n {}

printf '%s\n' '--- Workspace model ---'
sed -n '/^class Workspace\b/,/^class /p' backend/db/models.py

printf '%s\n' '--- Workspace provisioning service ---'
cat -n backend/services/workspace_scope.py

printf '%s\n' '--- Personal ownership acceptance ---'
cat -n backend/tests/test_workspace_personal_scope_binding_postgres.py

printf '%s\n' '--- 0021 migration acceptance candidate ---'
fd -a --type f 'test_*personal*binding*postgres.py' backend/tests -x sh -c 'echo "--- $1 ---"; cat -n "$1"' sh {}

printf '%s\n' '--- SQLAlchemy mutation shapes ---'
ast-grep run --lang python --pattern 'update(Workspace).where($$$WHERE).values($$$VALUES)' backend/services/workspace_scope.py || true
ast-grep run --lang python --pattern 'insert(Workspace).values($$$VALUES)' backend/services/workspace_scope.py || true
ast-grep run --lang python --pattern 'select(Workspace).where($$$WHERE)' backend/services/workspace_scope.py || true

Length of output: 38495


@seonghobae I reviewed exact head e204ea75d324809215c57de1fb389376039dc817 only.

I found one test coverage issue.

P2 — Add an upgrade-path acceptance test for historical personal workspaces

backend/tests/test_workspace_personal_scope_binding_postgres.py migrates a fresh database directly to head on Line 84. It does not test a database that has already reached 0020_workspace_organization_binding and contains a legacy personal workspace row with both organization_id and owner_user_id absent.

0021_workspace_personal_owner_binding states that it must leave historical rows unbound because no auditable owner exists. The fresh-install test cannot verify this requirement. It also cannot verify that adding owner_user_id, the index, and the single-scope check succeeds for pre-existing workspace_entities data.

Add a real PostgreSQL migration test that:

  1. Migrates to 0020_workspace_organization_binding.
  2. Inserts an unbound workspace row.
  3. Upgrades to head.
  4. Verifies that the row still has organization_id IS NULL and owner_user_id IS NULL.
  5. Re-runs the upgrade and verifies the same state.

The current concurrent CAS contract is sound by static inspection. The guarded update binds only rows where both scope fields are NULL. The persisted binding check rejects the losing claimant.

git diff --check and Python syntax parsing passed. I did not run the PostgreSQL acceptance suite.

You are interacting with an AI system.

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

Please review only exact head a74db4ea4e16d066502817f028d1710dc8d507be. The new delta strengthens the organization opaque-workspace regression so a historical NULL-document branch requires the same registry row to match workspace_id, organization_id, and owner_user_id IS NULL. Production ORM/API integration is intentionally still RED; predecessor approvals/checks must not be treated as evidence for this head.

@coderabbitai

coderabbitai Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

head="a74db4ea4e16d066502817f028d1710dc8d507be"
parent="$(git rev-parse "${head}^")"

printf '%s\n' '--- Exact revision identity ---'
git rev-parse HEAD
git show -s --format='commit=%H%nparent=%P%nsubject=%s%n%n%B' "$head"
test "$(git rev-parse HEAD)" = "$head"

printf '%s\n' '--- Changed paths and exact delta ---'
git diff --name-status "$parent" "$head"
git diff --check "$parent" "$head"
git diff --no-ext-diff --unified=100 "$parent" "$head"

printf '%s\n' '--- Changed Python outlines ---'
git diff --name-only "$parent" "$head" -- 'backend/**/*.py' |
while IFS= read -r file; do
  [ -n "$file" ] || continue
  printf '\n### %s\n' "$file"
  ast-grep outline "$file" --items all --view expanded || true
done

printf '%s\n' '--- Opaque organization regression and related acceptance tests ---'
fd -a --type f . backend/tests |
grep -E '(legacy_document_scope|workspace.*scope.*binding|workspace.*organization.*binding|personal.*scope)' |
sort |
while IFS= read -r file; do
  printf '\n### %s\n' "$file"
  rg -n -C 12 'opaque|tenant-space|_expected_organization_filter|organization_id|owner_user_id|Workspace|_document_organization_filter' "$file" || true
done

printf '%s\n' '--- Current authorization helpers and document queries ---'
sed -n '2460,2560p' backend/api/data.py
sed -n '3935,4020p' backend/api/data.py
rg -n -C 8 --glob 'backend/api/data.py' \
  'def (_get_workspace_document|_auth_context_owns_its_workspace|_document_organization_filter)|_document_organization_filter\(|Document\.(workspace_id|organization_id)|Workspace\.(workspace_id|organization_id|owner_user_id)' \
  backend/api/data.py

printf '%s\n' '--- Workspace model, migration chain, and provisioning service ---'
rg -n -A100 -B10 'class Workspace\b' backend/db/models.py
fd -a '0020_workspace_organization_binding.py|0021_workspace_personal_owner_binding.py' backend/alembic/versions -x sh -c 'echo "--- $1 ---"; cat -n "$1"' sh {}
fd -a 'workspace_scope.py' backend/services -x sh -c 'cat -n "$1"' sh {}

printf '%s\n' '--- All document-filter call sites and workspace persistence paths ---'
rg -n -C 6 --glob 'backend/**/*.py' \
  '_document_organization_filter\s*\(|get_or_create_scoped_workspace\s*\(|get_or_create_bound_workspace\s*\(|(insert|select|update)\(Workspace\)|Workspace\.(workspace_id|organization_id|owner_user_id)' \
  backend

printf '%s\n' '--- Static syntax parse and full PR whitespace check ---'
git diff --name-only "$parent" "$head" -- 'backend/**/*.py' | xargs -r python -m py_compile
base="$(git merge-base "$head" origin/develop 2>/dev/null || true)"
if [ -n "$base" ]; then git diff --check "$base...$head"; fi

Length of output: 95964


@seonghobae I reviewed only exact head a74db4ea4e16d066502817f028d1710dc8d507be.

I found no new actionable issues in the regression contract.

The revised compatibility branch requires one workspace_entities row that matches all of these conditions:

  • workspace_id == auth_context.workspace_id
  • organization_id == auth_context.organization_id
  • owner_user_id IS NULL

This condition prevents a personal-owner workspace row from authorizing access through the organization legacy-document branch. The negative contract still distinguishes two organizations that present the same opaque workspace ID.

The current ORM/API implementation does not yet satisfy this contract. This is the intentional source-order RED state that you specified.

git diff --check and Python syntax parsing passed. I did not run the intentionally failing tests.

You are interacting with an AI system.

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

Please review exact head 6d801e5c671366267c40c5346f547cf87fd8c45e only. This head is intentionally still RED in backend/api/data.py; focus on whether the new ORM ownership mapping and PostgreSQL endpoint/legacy regressions correctly encode the fail-closed opaque-workspace contract without identifier-shape ownership assumptions. Do not treat predecessor reviews or checks as current-head evidence.

@coderabbitai

coderabbitai Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

head="6d801e5c671366267c40c5346f547cf87fd8c45e"
parent="$(git rev-parse "${head}^")"

printf '%s\n' '--- Exact revision identity ---'
git rev-parse HEAD
git show -s --format='commit=%H%nparent=%P%nsubject=%s%n%n%B' "$head"
test "$(git rev-parse HEAD)" = "$head"

printf '%s\n' '--- Exact changed paths and source-order delta ---'
git diff --name-status "$parent" "$head"
git diff --check "$parent" "$head"
git diff --no-ext-diff --unified=100 "$parent" "$head"

printf '%s\n' '--- Changed Python outlines ---'
git diff --name-only "$parent" "$head" -- 'backend/**/*.py' |
while IFS= read -r file; do
  [ -n "$file" ] || continue
  printf '\n### %s\n' "$file"
  ast-grep outline "$file" --items all --view expanded || true
done

printf '%s\n' '--- Workspace ORM mapping and migration chain ---'
rg -n -C 18 --glob 'backend/db/models.py' \
  'class Workspace\b|class Document\b|organization_id|owner_user_id' backend/db/models.py
rg -n -C 4 --glob 'backend/alembic/versions/*.py' \
  '^(revision|down_revision|branch_labels|depends_on)\s*=' backend/alembic/versions
fd -a '0020_workspace_organization_binding.py|0021_workspace_personal_owner_binding.py' backend/alembic/versions -x sh -c '
  echo "--- $1 ---"
  cat -n "$1"
' sh {}

printf '%s\n' '--- Workspace provisioning and API ownership mapping ---'
fd -a 'workspace_scope.py' backend/services -x sh -c 'echo "--- $1 ---"; cat -n "$1"' sh {}
sed -n '2440,2585p' backend/api/data.py
sed -n '3120,3420p' backend/api/data.py
sed -n '3920,4035p' backend/api/data.py
rg -n -C 8 --glob 'backend/**/*.py' \
  'get_or_create_scoped_workspace\s*\(|get_or_create_bound_workspace\s*\(|_document_organization_filter\s*\(|_get_workspace_document\s*\(|Workspace\.(workspace_id|organization_id|owner_user_id)|Document\.(workspace_id|organization_id)' \
  backend

printf '%s\n' '--- Changed PostgreSQL tests ---'
git diff --name-only "$parent" "$head" -- 'backend/tests/**/*.py' |
while IFS= read -r file; do
  [ -n "$file" ] || continue
  printf '\n### %s\n' "$file"
  cat -n "$file"
done

printf '%s\n' '--- Related endpoint and legacy document test contracts ---'
fd -a --type f . backend/tests |
grep -E '(legacy_document_scope|workspace.*scope.*binding|personal.*scope|data_api)' |
sort |
while IFS= read -r file; do
  printf '\n### %s\n' "$file"
  rg -n -C 10 'opaque|tenant-space|owner_user_id|organization_id|Workspace|workspace_id|quality_surface|reparse|documents' "$file" || true
done

printf '%s\n' '--- Static syntax parse and exact whitespace check ---'
git diff --name-only "$parent" "$head" -- 'backend/**/*.py' | xargs -r python -m py_compile
git diff --check "$parent" "$head"

Length of output: 236903


@seonghobae I reviewed only exact head 6d801e5c671366267c40c5346f547cf87fd8c45e.

I found no new actionable issues.

The changed PostgreSQL endpoint regression correctly uses an opaque workspace ID. It persists one organization-bound workspace_entities row with owner_user_id set to NULL. It then verifies both required outcomes:

  • The matching organization can reparse and list the legacy workspace_documents.organization_id IS NULL document.
  • A different organization with the same workspace ID receives 404 and cannot list the document.

This contract does not use workspace identifier spelling as ownership evidence. It requires the persisted registry binding instead.

The current _document_organization_filter() does not meet this contract. It still uses _auth_context_owns_its_workspace(). Therefore, the regression remains correctly source-order RED.

git diff --check and Python syntax parsing passed. I did not run the PostgreSQL regression suite.

You are interacting with an AI system.

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

현재 exact head 602c1447d781583d70ae7f96846810e89e93e5ad만 기준으로 검토해 주세요. 이전 head의 review/check receipt는 승계하지 않습니다. 특히 persisted workspace owner EXISTS 상관관계, HMAC consume-only 경계, deterministic 403 처리와 새 source change가 기존 migration/endpoint acceptance 계약을 넓히거나 약화하지 않는지 확인해 주세요. backend/tests/test_data_api.py의 기존 HMAC success fixture가 아직 unbound workspace를 가정하는 것은 현재 알려진 RED이며, production policy를 완화하는 방식의 수정은 허용하지 않습니다.

@coderabbitai

coderabbitai Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

head="602c1447d781583d70ae7f96846810e89e93e5ad"
parent="$(git rev-parse "${head}^")"

printf '%s\n' '--- Exact revision identity ---'
git rev-parse HEAD
git show -s --format='commit=%H%nparent=%P%nsubject=%s%n%n%B' "$head"
test "$(git rev-parse HEAD)" = "$head"

printf '%s\n' '--- Exact changed paths and source-order delta ---'
git diff --name-status "$parent" "$head"
git diff --check "$parent" "$head"
git diff --no-ext-diff --unified=100 "$parent" "$head"

printf '%s\n' '--- Changed Python outlines ---'
git diff --name-only "$parent" "$head" -- 'backend/**/*.py' |
while IFS= read -r file; do
  [ -n "$file" ] || continue
  printf '\n### %s\n' "$file"
  ast-grep outline "$file" --items all --view expanded || true
done

printf '%s\n' '--- Workspace model and provisioning service ---'
sed -n '/^class Workspace\b/,/^class /p' backend/db/models.py
fd -a 'workspace_scope.py' backend/services -x sh -c 'echo "--- $1 ---"; cat -n "$1"' sh {}

printf '%s\n' '--- Data API authorization and document endpoint paths ---'
sed -n '2440,2590p' backend/api/data.py
sed -n '3120,3440p' backend/api/data.py
sed -n '3920,4045p' backend/api/data.py
rg -n -C 8 --glob 'backend/api/data.py' \
  'def (_document_organization_filter|_get_workspace_document)|Workspace\.(workspace_id|organization_id|owner_user_id)|Document\.(workspace_id|organization_id)|WorkspaceOrganizationConflict|HTTPException.*403|get_or_create_scoped_workspace' \
  backend/api/data.py

printf '%s\n' '--- Exact changed tests and related acceptance contracts ---'
git diff --name-only "$parent" "$head" -- 'backend/tests/**/*.py' |
while IFS= read -r file; do
  [ -n "$file" ] || continue
  printf '\n### %s\n' "$file"
  cat -n "$file"
done
fd -a --type f . backend/tests |
grep -E '(legacy_document_scope|workspace.*scope.*binding|personal.*scope|workspace.*organization.*binding|data_api)' |
sort |
while IFS= read -r file; do
  printf '\n### Contract references: %s\n' "$file"
  rg -n -C 8 'opaque|owner_user_id|organization_id|HMAC|forbidden|403|WorkspaceOrganizationConflict|workspace_id|quality_surface|reparse' "$file" || true
done

printf '%s\n' '--- Migration chain and workspace migrations ---'
rg -n -C 4 --glob 'backend/alembic/versions/*.py' \
  '^(revision|down_revision|branch_labels|depends_on)\s*=' backend/alembic/versions
fd -a '0020_workspace_organization_binding.py|0021_workspace_personal_owner_binding.py' backend/alembic/versions -x sh -c 'echo "--- $1 ---"; cat -n "$1"' sh {}

printf '%s\n' '--- Workspace and document database operation candidates ---'
rg -n -C 6 --glob 'backend/**/*.py' \
  '(insert|select|update)\(Workspace\)|Workspace\.(workspace_id|organization_id|owner_user_id)|get_or_create_scoped_workspace\s*\(|_document_organization_filter\s*\(' \
  backend

printf '%s\n' '--- Static syntax and exact whitespace checks ---'
git diff --name-only "$parent" "$head" -- 'backend/**/*.py' | xargs -r python -m py_compile
git diff --check "$parent" "$head"

Length of output: 231436


🏁 Script executed:

#!/bin/bash
set -euo pipefail

head="602c1447d781583d70ae7f96846810e89e93e5ad"
parent="$(git rev-parse "${head}^")"

printf '%s\n' '--- AuthContext definition and construction paths ---'
ast-grep outline backend/api/auth.py --items all --view expanded
rg -n -C 12 --glob 'backend/api/auth.py' \
  'class AuthContext\b|session_verifier|AuthContext\(|def get_auth_context|def get_current_user|hmac|oidc|override|server' \
  backend/api/auth.py

printf '%s\n' '--- Every Data API document route and scoped-workspace invocation ---'
rg -n -C 10 --glob 'backend/api/data.py' \
  '`@router`\.(get|post|put|patch|delete)\("/documents|`@router`\.(get|post|put|patch|delete)\("/quality-surface|_require_scoped_workspace\(|_get_workspace_document\(' \
  backend/api/data.py

printf '%s\n' '--- Exact changed-line source with line numbers ---'
git diff --unified=12 "$parent" "$head" -- backend/api/data.py | cat

printf '%s\n' '--- Verifier provenance contracts and API tests that exercise 403 ---'
rg -n -C 10 --glob 'backend/tests/**/*.py' \
  'session_verifier|Workspace access denied|status_code == 403|WorkspaceOrganizationBindingRequired|WorkspaceOrganizationConflict|upload_data_document|pdf-dom-recognition' \
  backend/tests

printf '%s\n' '--- Test fixture workspace setup and mock update support ---'
sed -n '55,185p' backend/tests/test_data_api.py
sed -n '300,430p' backend/tests/test_data_api.py
sed -n '2520,3025p' backend/tests/test_data_api.py

printf '%s\n' '--- Changed module syntax and whitespace ---'
python -m py_compile backend/api/data.py
git diff --check "$parent" "$head"

Length of output: 169408


@seonghobae exact head 602c1447d781583d70ae7f96846810e89e93e5ad만 검토했습니다.

P1 — 실제 인증 경로에서 personal workspace scope를 허용해야 합니다

_require_scoped_workspace()는 Line 2499에서 auth_context.organization_id를 전달합니다. _document_organization_filter()의 personal workspace 분기는 Line 2524부터 organization_id is None을 처리합니다.

하지만 backend/api/auth.py_auth_context_from_session_payload()는 Line 518부터 Line 519에서 org claim이 없으면 401을 반환합니다. 따라서 실제 get_auth_context 인증 경로에서는 organization_id=NoneAuthContext가 생성되지 않습니다.

이 상태에서는 personal opaque workspace의 owner binding, HMAC consume-only 재사용, 그리고 personal legacy NULL document 접근이 HTTP Data API에서 도달 불가능합니다. 현재 acceptance는 직접 함수 호출과 dependency override로만 이 경로를 실행합니다.

personal workspace를 지원하는 정책이면, 인증 claim 검증이 org 없는 personal session을 유효하게 만들고 해당 session이 owner_user_id binding 경로에 도달하도록 수정해야 합니다. 조직 scope의 fail-closed 정책은 완화하면 안 됩니다.

organization scope의 EXISTSworkspace_id, organization_id, owner_user_id IS NULL을 한 Workspace 행에서 확인합니다. 이 상관관계는 올바릅니다. HMAC은 새 binding을 만들거나 변경하지 않습니다. 두 upload endpoint는 binding 실패를 동일한 403 Workspace access denied 응답으로 변환합니다.

git diff --checkpython -m py_compile backend/api/data.py는 통과했습니다. PostgreSQL acceptance는 이 환경에서 실행하지 않았습니다.

You are interacting with an AI system.

@seonghobae
seonghobae dismissed coderabbitai[bot]’s stale review September 15, 2026 00:46

Historical predecessor-head review only. All inline findings from this review are resolved; the branch has since advanced through persisted workspace-ownership integration and HMAC fixture repair. Dismissing this stale blocking state does not satisfy the review gate: exact head f7bdcf1 still requires a fresh qualifying independent review before Ready/merge.

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

Please review only exact head f7bdcf11c0ddf0bdfb04a28da18d2d06aa710d67. Focus on the persisted workspace-owner invariant, HMAC consume-only behavior, and the new test-fixture binding in backend/tests/conftest.py. Do not treat predecessor review receipts as current-head evidence.

@coderabbitai

coderabbitai Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

head="f7bdcf11c0ddf0bdfb04a28da18d2d06aa710d67"
parent="$(git rev-parse "${head}^")"

printf '%s\n' '--- Exact revision identity ---'
git rev-parse HEAD
git show -s --format='commit=%H%nparent=%P%nsubject=%s%n%n%B' "$head"
test "$(git rev-parse HEAD)" = "$head"

printf '%s\n' '--- Exact changed paths and source-order delta ---'
git diff --name-status "$parent" "$head"
git diff --check "$parent" "$head"
git diff --no-ext-diff --unified=100 "$parent" "$head"

printf '%s\n' '--- Changed Python outlines ---'
git diff --name-only "$parent" "$head" -- 'backend/**/*.py' |
while IFS= read -r file; do
  [ -n "$file" ] || continue
  printf '\n### %s\n' "$file"
  ast-grep outline "$file" --items all --view expanded || true
done

printf '%s\n' '--- Test fixture binding and dependent test fixtures ---'
fd -a 'conftest.py' backend/tests -x sh -c '
  echo "--- $1 ---"
  rg -n -C 16 "workspace|Workspace|organization_id|owner_user_id|AuthContext|session_verifier|HMAC|hmac" "$1" || true
' sh {}

printf '%s\n' '--- Workspace model and provisioning service ---'
rg -n -C 18 --glob 'backend/db/models.py' \
  'class Workspace\b|organization_id|owner_user_id' backend/db/models.py
fd -a 'workspace_scope.py' backend/services -x sh -c '
  echo "--- $1 ---"
  cat -n "$1"
' sh {}

printf '%s\n' '--- API authorization and provisioning call sites ---'
rg -n -C 10 --glob 'backend/api/data.py' \
  'def (_require_scoped_workspace|_document_organization_filter|_get_workspace_document)|get_or_create_scoped_workspace|WorkspaceOrganization|Workspace\.(workspace_id|organization_id|owner_user_id)|Document\.(workspace_id|organization_id)' \
  backend/api/data.py

printf '%s\n' '--- Auth verifier provenance and AuthContext construction ---'
rg -n -C 12 --glob 'backend/api/auth.py' \
  'class AuthContext\b|AuthContext\(|session_verifier|HMAC|hmac|organization_id|owner_user_id|user_id' \
  backend/api/auth.py

printf '%s\n' '--- Persisted workspace binding operations and test coverage ---'
rg -n -C 8 --glob 'backend/**/*.py' \
  'get_or_create_scoped_workspace\s*\(|WorkspaceOrganizationBindingRequired|WorkspaceOrganizationConflict|WorkspaceOwner|owner_user_id|organization_id.*workspace_entities|workspace_entities.*organization_id' \
  backend

printf '%s\n' '--- Relevant API and PostgreSQL test contracts ---'
fd -a --type f . backend/tests |
grep -E '(data_api|workspace.*scope|personal.*scope|legacy_document_scope)' |
sort |
while IFS= read -r file; do
  printf '\n### %s\n' "$file"
  rg -n -C 8 'HMAC|hmac|opaque|owner_user_id|organization_id|Workspace|workspace_id|403|binding|session_verifier' "$file" || true
done

printf '%s\n' '--- Static syntax and whitespace checks ---'
git diff --name-only "$parent" "$head" -- 'backend/**/*.py' | xargs -r python -m py_compile
git diff --check "$parent" "$head"

Length of output: 241865


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- mock_db fixture definition and nearby fixture scopes ---'
rg -n -A40 -B8 --glob 'backend/tests/test_data_api.py' \
  'def mock_db\b|`@pytest`\.fixture.*mock_db|MockAsyncSession\(' backend/tests/test_data_api.py

printf '%s\n' '--- First target HMAC upload test ---'
rg -n -A70 -B12 --glob 'backend/tests/test_data_api.py' \
  '^def test_data_document_upload_creates_workspace_scoped_document\b' backend/tests/test_data_api.py

printf '%s\n' '--- Second target HMAC upload test ---'
rg -n -A75 -B12 --glob 'backend/tests/test_data_api.py' \
  '^def test_data_pdf_dom_upload_persists_signed_organization_scope\b' backend/tests/test_data_api.py

printf '%s\n' '--- All tests that use each endpoint with signed HMAC setup ---'
rg -n -C 5 --glob 'backend/tests/test_data_api.py' \
  '_with_signed_auth\(|/api/data/documents"|/api/data/documents/' backend/tests/test_data_api.py

printf '%s\n' '--- Fixture resolution order references ---'
rg -n -C 4 --glob 'backend/tests/**/*.py' \
  'persisted_hmac_workspace_for_document_uploads|dev_auth_dependency_overrides|mock_db' backend/tests

Length of output: 99609


@seonghobae I reviewed only exact head f7bdcf11c0ddf0bdfb04a28da18d2d06aa710d67.

I found one issue.

P1 — Prebind the HMAC action test or make its document organization-scoped

backend/tests/test_data_api.py::test_data_document_actions_are_workspace_scoped_and_intent_only still creates doc_owned with organization_id=None. It then uses an HMAC session and expects successful reparse, embedding, and HWP operations.

For a legacy NULL-organization document, _document_organization_filter() requires a persisted workspace_entities row that matches workspace_id, organization_id, and owner_user_id IS NULL. The new autouse fixture does not run for this test.

MockAsyncSession does not evaluate the Workspace EXISTS predicate. It accepts Document.organization_id is None for an organization-scoped query. Therefore, this test can pass even though production must return 404 when the registry binding is absent.

Add this test to _HMAC_DOCUMENT_UPLOAD_COMPATIBILITY_TESTS or rename that set to cover all HMAC compatibility consumers. Alternatively, set doc_owned.organization_id="org-acme" if the test does not need to exercise legacy-document compatibility. Keep the HMAC consume-only policy unchanged.

The two newly targeted upload tests correctly use signed HMAC sessions and receive the matching persisted organization binding. The binding has owner_user_id=None, so it preserves the organization-workspace invariant. git diff --check and Python syntax parsing passed.

You are interacting with an AI system.

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

Please review only exact head 9151c75568c582c8147cfee6757cd00a9b4d60b7. The previous exact-head review found the legacy NULL-organization document-action HMAC success case lacked persisted workspace-owner evidence; this head adds that same pre-bound registry prerequisite without changing production authorization. Do not transfer predecessor review evidence.

@coderabbitai

coderabbitai Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

head="9151c75568c582c8147cfee6757cd00a9b4d60b7"
parent="$(git rev-parse "${head}^")"

printf '%s\n' '--- Exact revision identity ---'
git rev-parse HEAD
git show -s --format='commit=%H%nparent=%P%nsubject=%s%n%n%B' "$head"
test "$(git rev-parse HEAD)" = "$head"

printf '%s\n' '--- Exact changed paths and source-order delta ---'
git diff --name-status "$parent" "$head"
git diff --check "$parent" "$head"
git diff --no-ext-diff --unified=100 "$parent" "$head"

printf '%s\n' '--- Changed Python outlines ---'
git diff --name-only "$parent" "$head" -- 'backend/**/*.py' |
while IFS= read -r file; do
  [ -n "$file" ] || continue
  printf '\n### %s\n' "$file"
  ast-grep outline "$file" --items all --view expanded || true
done

printf '%s\n' '--- Target action test and compatibility fixture selection ---'
rg -n -A110 -B18 --glob 'backend/tests/test_data_api.py' \
  'test_data_document_actions_are_workspace_scoped_and_intent_only|_HMAC_DOCUMENT_UPLOAD_COMPATIBILITY_TESTS|persisted_hmac_workspace_for_document_uploads|MockAsyncSession|def mock_db\b' \
  backend/tests/test_data_api.py

printf '%s\n' '--- Shared fixture binding ---'
fd -a 'conftest.py' backend/tests -x sh -c '
  echo "--- $1 ---"
  rg -n -C 25 "persisted_hmac_workspace_for_document_uploads|HMAC_DOCUMENT|workspace_entities|Workspace\\(|workspace_id|organization_id|owner_user_id" "$1" || true
' sh {}

printf '%s\n' '--- Production authorization and workspace model ---'
sed -n '2450,2575p' backend/api/data.py
sed -n '/^class Workspace\b/,/^class /p' backend/db/models.py
fd -a 'workspace_scope.py' backend/services -x sh -c 'cat -n "$1"' sh {}

printf '%s\n' '--- Relevant query emulation and all fixture consumers ---'
rg -n -C 12 --glob 'backend/tests/**/*.py' \
  'Document\.organization_id|Workspace\.organization_id|Workspace\.owner_user_id|persisted_hmac_workspace_for_document_uploads|_HMAC_DOCUMENT_UPLOAD_COMPATIBILITY_TESTS' \
  backend/tests

printf '%s\n' '--- Static syntax and whitespace checks ---'
git diff --name-only "$parent" "$head" -- 'backend/**/*.py' | xargs -r python -m py_compile
git diff --check "$parent" "$head"

Length of output: 51960


@seonghobae I reviewed only exact head 9151c75568c582c8147cfee6757cd00a9b4d60b7.

I found one issue.

P1 — Make the mock enforce the pre-bound registry requirement

The new autouse fixture adds the correct Workspace row for test_data_document_actions_are_workspace_scoped_and_intent_only. The row matches workspace_id="workspace-org-acme", organization_id="org-acme", and owner_user_id=None.

However, MockAsyncSession.execute() still accepts a document with Document.organization_id is None whenever the requested organization is org-acme. It does not evaluate the correlated Workspace EXISTS predicate in _document_organization_filter().

As a result, removing the new fixture would not change the action test result. The test would still return 200 for the legacy NULL-organization document. The test therefore does not prove that HMAC access consumes persisted owner evidence.

Make the mock require a matching workspace row before it returns a legacy NULL-organization document for an organization-scoped request. The required row must match the workspace ID, organization ID, and owner_user_id is None. Alternatively, add this endpoint contract to a real PostgreSQL test. Keep the HMAC consume-only policy unchanged.

git diff --check and syntax parsing of backend/tests/conftest.py passed. I did not run the test suite.

You are interacting with an AI system.

Copy link
Copy Markdown
Contributor Author

#1691 authority advanced after a verified current-head review finding. The repository-CI prerequisite is now fix/stacked-pr-trigger-foundation@f985a00030028c9989637b3fafffac07d95e2de2, not 971f1752....

Verified repair: the PostgreSQL CI contract was outside Application CI's only pytest collection (cd backend && python -m pytest -q). #1691 moved it non-destructively to backend/tests/test_postgres_ci_contract.py, changed REPO_ROOT to parents[2], removed the obsolete root copy, resolved the outdated inline thread, and dismissed the predecessor-head CHANGES_REQUESTED review as historical only. Fresh current-head review and terminal workflows are still required; the six repo-local workflows on f985a000... are currently queued.

#1503 remains Draft at 9151c75568c582c8147cfee6757cd00a9b4d60b7. Do not copy #1691 source here or transfer predecessor evidence. Adopt the CI foundation only after normal protected integration, then obtain #1503's own unchanged-head focused/real-PostgreSQL/hosted GREEN and independent review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working priority: high High-priority or P1 work status: draft Draft pull request type: bug Defect or incorrect behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants